Skip to content

fs: improve error performance for writeSync #50354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fs: improve error performance for writeSync #50354

wants to merge 2 commits into from

Conversation

@pluris
Copy link
Contributor

@pluris pluris commented Oct 24, 2023 •
edited
Loading

I worked on it because it seemed like there was still work left to do in nodejs/performance#106.

                                              confidence improvement accuracy (*)    (**)   (***)
fs/bench-writeSync.js n=100000 type='invalid'        ***    116.41 %       ±9.35% ±12.49% ±16.36%
fs/bench-writeSync.js n=100000 type='valid'                   0.46 %       ±6.13%  ±8.16% ±10.62%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Refs: nodejs/performance#106

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. typings labels Oct 24, 2023
@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. performance Issues and PRs related to the performance of Node.js. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55187/

@anonrig
Copy link
Member

anonrig commented Oct 24, 2023

@pluris the tests are failing

@pluris
Copy link
Contributor Author

pluris commented Oct 24, 2023

@anonrig After rebasing and checking, there seems to be a problem. I will fix it and let you know. thank you

@pluris
Copy link
Contributor Author

pluris commented Oct 24, 2023 •
edited
Loading

Something is weird. It seems like a fail occurs in parallel.test-release-changelog.
It seems to be happening in other PR as well. (#50361)
Could it be related to the recent commit that changed the CHANGELOG file?

@richardlau
Copy link
Member

Something is weird. It seems like a fail occurs in parallel.test-release-changelog. It seems to be happening in other PR as well. (#50361) Could it be related to the recent commit that changed the CHANGELOG file?

#50373. Being fixed by #50375. Will need a fresh (not resumed) CI run to pick up the change when it lands on main.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55210/

@github-actions github-actions bot mentioned this pull request Oct 25, 2023
Open
16 tasks
@H4ad H4ad added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55241/

This was referenced Oct 26, 2023
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56175/

@pluris pluris force-pushed the feat/write_sync branch 2 times, most recently from 3824b3a to 0d46fd4 Compare December 10, 2023 14:23
@deokjinkim deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56521/

@pluris pluris force-pushed the feat/write_sync branch 4 times, most recently from e08d243 to 1055136 Compare December 25, 2023 09:55
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56550/

@pluris pluris force-pushed the feat/write_sync branch 3 times, most recently from d68e8ef to 8f8be72 Compare December 27, 2023 16:22
@pluris pluris closed this Dec 29, 2023
@pluris pluris reopened this Dec 29, 2023
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2023
@aduh95
Copy link
Contributor

aduh95 commented Dec 30, 2023 •
edited
Loading

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1490/

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56597/

@PodaruDragos
Copy link

hey guys, is there anything that's blocking this one out ?

@pluris pluris closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fhinkel fhinkel fhinkel approved these changes

@anonrig anonrig anonrig approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. typings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.